feat: sync with exam service on course publish by zacharis278 · Pull Request #31015 · openedx/openedx-platform

zacharis278 · 2022-09-19T12:56:30Z

Description

Call into the exam service instead of the edx-proctoring plugin on course publish. This feature is currently behind the course_apps.exams_ida course waffle flag.

configuration PR: https://github.com/edx/edx-internal/pull/7120

zacharis278 · 2022-09-19T17:32:49Z

cms/djangoapps/contentstore/exams.py

+User = get_user_model()
+
+
+def register_exams(course_key):


there is some intentional code duplication between this function and the edx-proctoring register_special_exams. I think it would be messier to try to reuse some of these statements when these two integrations may evolve differently.

ashultz0 · 2022-09-19T17:45:31Z

cms/djangoapps/contentstore/tasks.py

    LOGGER.info('Attempting to register exams for course %s', course_key_str)
+
+    # Call the appropriate handler for either the exams IDA or the edx-proctoring plugin
+    register_exams_handler = register_exams if exams_ida_enabled(course_key) else register_exams_legacy


I like the naming and structure here, it is very clear about what is what in very little space

ashultz0 · 2022-09-19T17:46:53Z

cms/djangoapps/contentstore/exams.py

+
+    try:
+        _patch_course_exams(exams_list, str(course_key))
+        log.info(f'Successfully registered {locations} with exam service')


are the locations useful in this log or too detailed? what about the course key?

It doesn't actually come out to messy since I wouldn't expect a large number of exams in one course Successfully registered ['block-v1:edX+DemoX+Demo_Course+type@sequential+block@00a45f2d4cc54b07a172183a1834d710', 'block-v1:edX+DemoX+Demo_Course+type@sequential+block@00a45f2d4cc54b07a172183a1834d71123'] with exam service

But you might have a point, I'm not sure if this is actually more useful than just the course key 🤔

varshamenon4

Looks great! Approving as I just had some clarifying questions. :)

varshamenon4 · 2022-09-19T21:07:09Z

cms/djangoapps/contentstore/exams.py

+        }
+    )
+
+    # filter out any potential dangling sequences


[question]: @zacharis278, could you clarify what is happening here and what is meant by "dangling sequences"? Thanks!

A dangling sequence is one that is part of the course but has no relationship back up to the course block. If you look at the test for this it's a kind of easier to see. For example, the section that is a parent of the sequence is deleted.

varshamenon4 · 2022-09-19T21:09:14Z

cms/djangoapps/contentstore/exams.py

+        log.info(msg)
+        locations.append(location)
+
+        if timed_exam.is_proctored_exam:


[question/suggestion]: Could there be any potential issues in the future with using conditional statements like this and also with defining the exam types here? Is there an enum/class/other data type that could be better used here?

oh good observation, my thinking is we would eventually move away from using these independent variables and just directly set exam type. They don't serve any purpose in the LMS/CMS other than backwards compatibility. Maybe I should move this into a function just in case it needs to be referenced elsewhere.

varshamenon4 · 2022-09-19T21:10:48Z

cms/djangoapps/contentstore/exams.py

+    return client
+
+
+def _patch_course_exams(exams_list, course_id):


I like that you've created a helper method for this! Looks good.

varshamenon4 · 2022-09-19T21:11:10Z

cms/djangoapps/contentstore/tasks.py

    LOGGER.info('Attempting to register exams for course %s', course_key_str)
+
+    # Call the appropriate handler for either the exams IDA or the edx-proctoring plugin
+    register_exams_handler = register_exams if exams_ida_enabled(course_key) else register_exams_legacy


varshamenon4 · 2022-09-19T21:12:20Z

cms/djangoapps/contentstore/tests/test_exams.py

+@override_waffle_flag(EXAMS_IDA, active=True)
+@patch.dict('django.conf.settings.FEATURES', {'ENABLE_SPECIAL_EXAMS': True})
+@patch('cms.djangoapps.contentstore.exams._patch_course_exams')
+class TestExamService(ModuleStoreTestCase):


Nice test coverage!

edx-pipeline-bot · 2022-09-20T17:46:30Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-09-20T18:15:38Z

EdX Release Notice: This PR has been deployed to the production environment.

ormsbee · 2022-10-25T16:23:41Z

cms/djangoapps/contentstore/tests/test_exams.py

+    """
+    Test for syncing exams to the exam service
+    """
+    MODULESTORE = TEST_DATA_MONGO_AMNESTY_MODULESTORE


@zacharis278: I ran across this code while reviewing #31134

Please note that this is testing against Old Mongo, the Modulestore that's been deprecated for years and is currently in the process of being removed. Please use TEST_DATA_SPLIT_MODULESTORE (I believe it will default to this without the override line here).

@ormsbee oh weird, yeah looks like I just blindly copied this from the older proctoring tests. Is cleaning this up blocking anything?

@zacharis278: That PR I linked to above is removing features from Old Mongo that this test depends on. In general, the team has tried to convert Old Mongo modulestore based tests to Split Modulestore based ones. Sometimes, a straightforward conversion fails (like this one, apparently). The policy we've adopted for tests that look useful but have never worked in Split Mongo is to mark them as @skip("OldMongo Deprecation") and review them later to see if they're worth keeping.

I realize this is new, actively developed functionality, and not some old test written 8+ years ago. I really hope it's a straightforward change for you folks. But we've been as loud as we could for a long while that Old Mongo is going away. So fixing this is non-blocking in the sense that we can land #31134 without it, but we would do that by disabling the test.

In terms of the timing, I don't think we'll merge before next week. It's a big PR that touches a bunch of old functionality, some of which I had forgotten even existed. I've been looking over for hours and I'm still less than halfway through it. Even assuming very quick turnaround on review cycles, I wouldn't expect the reviews to be done before Friday, and we wouldn't release something like this that close to the weekend.

If there's proctoring code that relies on using Old Mongo, that should definitely get looked at as well. There was at least one that was a straightforward conversion (cms/djangoapps/contentstore/tests/test_proctoring.py), but I don't know if that was the one you were referring to.

zacharis278 marked this pull request as ready for review September 19, 2022 14:09

zacharis278 force-pushed the zhancock/publish-exams branch 2 times, most recently from b272620 to 8f7c7ac Compare September 19, 2022 17:16

zacharis278 commented Sep 19, 2022

View reviewed changes

ashultz0 reviewed Sep 19, 2022

View reviewed changes

ashultz0 approved these changes Sep 19, 2022

View reviewed changes

varshamenon4 approved these changes Sep 19, 2022

View reviewed changes

zacharis278 added 5 commits September 20, 2022 11:31

feat: sync with exam service on course publish

e3b03a1

style: self-review tweaks

5f74f0f

test: fixup

c1e4d52

feat: update exams worker name

d946dfa

style: move exam type translation to function

379b9e4

zacharis278 force-pushed the zhancock/publish-exams branch from 06b3c2b to 379b9e4 Compare September 20, 2022 15:32

zacharis278 merged commit 2f3c93e into master Sep 20, 2022

zacharis278 deleted the zhancock/publish-exams branch September 20, 2022 16:38

ormsbee reviewed Oct 25, 2022

View reviewed changes

		return client


		def _patch_course_exams(exams_list, course_id):

Conversation

zacharis278 commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varshamenon4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edx-pipeline-bot commented Sep 20, 2022

Uh oh!

edx-pipeline-bot commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zacharis278 commented Sep 19, 2022 •

edited

Loading